Fix handling of items inside a `doc(hidden)` block #107000

GuillaumeGomez · 2023-01-17T19:42:44Z

src/librustdoc/clean/mod.rs

src/librustdoc/passes/strip_hidden.rs

GuillaumeGomez · 2023-01-17T20:12:14Z

tests/rustdoc/redirect.rs

-// @has - '//p/a' '../../reexp_stripped/struct.Bar.html'
+// `Bar` is in a `doc(hidden)` module so the link should point to a non-existing file.
+// However, the struct is reexported at the root of the crate so we should point to it.
+// @has - '//a[@href="../reexp_stripped/struct.Bar.html"]' 'Bar'


Currently this test is failing. I need to find out why we generate a link to the original location of the item and not to the reexport directly.

GuillaumeGomez · 2023-01-17T23:16:40Z

So now just remains to fix the mystery of the bad item linking.

aDotInTheVoid · 2023-01-18T02:58:49Z

tests/rustdoc/hidden-private.rs

+    }
+
+    // @has 'foo/struct.Private.html'
+    // @!has - '//*[@id="impl-Bar-for-Private"]/*[@class="code-header"]' 'impl Bar for Private'


Why is this hidden? It means that If have a private type, and use #[derive(Serialize)], the impl won't appear with --document-private-items, which seems undesirable.

This is exactly the discussion we had here. If something is inside a hidden item's descendants, then it doesn't appear (not private, hidden).

tests/rustdoc/hidden-private.rs

GuillaumeGomez · 2023-01-18T22:26:18Z

So I just pushed a new commit which changes a bit in case the item is reexported how its URL is computed. This is a change that needs to be very carefully reviewed. I'm not sure if this was the right approach to the problem so if someone has another idea in mind that allows to not change how some URLs are generated, it'd be really awesome!

GuillaumeGomez · 2023-01-19T13:52:48Z

Fixed the bug! \o/

The problem was that I was removing the item with #[doc(hidden)] and all its children. However, the children should have been stripped, not removed.

GuillaumeGomez · 2023-01-19T16:11:21Z

Ok, I forgot to recurse into hidden children. Fixed that too.

…iddle rustdoc: Revert rust-lang#104889 Reverts rust-lang#104889. I don't think I'll be able to finish rust-lang#107000 on time unfortunately so to prevent rust-lang#106373, better to revert it and to make it into the next release. r? `@notriddle`

…handled as expected

GuillaumeGomez · 2023-01-27T22:57:32Z

The implementation is now in line with the rest of the implementation blocks handling in rustdoc.

Ready for new review round.

GuillaumeGomez · 2023-01-30T17:57:03Z

Forgot to change status...

@bors ready

notriddle · 2023-02-02T17:21:47Z

@bors r=notriddle,petrochenkov rollup=never

bors · 2023-02-02T17:21:49Z

📌 Commit ea84418 has been approved by notriddle,petrochenkov

It is now in the queue for this repository.

bors · 2023-02-02T21:14:48Z

⌛ Testing commit ea84418 with merge 6c991b0...

bors · 2023-02-03T00:31:25Z

☀️ Test successful - checks-actions
Approved by: notriddle,petrochenkov
Pushing 6c991b0 to master...

rust-timer · 2023-02-03T01:51:17Z

Finished benchmarking commit (6c991b0): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	4.1%	[0.2%, 18.9%]	9
Regressions ❌ (secondary)	1.1%	[0.2%, 1.7%]	5
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	4.1%	[0.2%, 18.9%]	9

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.3%	[1.4%, 3.5%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-3.2%, -1.5%]	2
All ❌✅ (primary)	2.3%	[1.4%, 3.5%]	3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	7.9%	[3.9%, 16.6%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	7.9%	[3.9%, 16.6%]	4

notriddle · 2023-02-03T02:43:47Z

This bug isn’t going to be fixed without some kind of perf regression, because it just has to walk through more of the AST. It happened last time for the same reason. #104889 (comment)

pnkfelix · 2023-02-07T17:34:59Z

Agree with @notriddle

@rustbot label: +perf-regression-triaged

rustbot assigned notriddle Jan 17, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 17, 2023

GuillaumeGomez force-pushed the fix-items-in-doc-hidden-block branch from 6184894 to 6921740 Compare January 17, 2023 19:43

fmease reviewed Jan 17, 2023

View reviewed changes

src/librustdoc/clean/mod.rs Outdated Show resolved Hide resolved

GuillaumeGomez force-pushed the fix-items-in-doc-hidden-block branch from 6921740 to 2db19cd Compare January 17, 2023 20:08

GuillaumeGomez commented Jan 17, 2023

View reviewed changes

src/librustdoc/passes/strip_hidden.rs Outdated Show resolved Hide resolved

GuillaumeGomez commented Jan 17, 2023

View reviewed changes